Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store information about each proxy in ModuleExtensionUsage #22307

Closed
wants to merge 2 commits into from

Conversation

Wyverald
Copy link
Member

@Wyverald Wyverald commented May 9, 2024

The current ModuleExtensionUsage class was designed in the days before we allowed multiple use_extension calls for the same extension in the same module. This PR brings the class up to speed to better reflect the actual API.

This will also lay the groundwork for bazel mod tidy working with include()d segments.

Work towards #22063

The current ModuleExtensionUsage class was designed in the days before we allowed multiple `use_extension` calls for the same extension in the same module. This PR brings the class up to speed to better reflect the actual API.

This will also lay the groundwork for `bazel mod tidy` working with `include()`d segments.

Work towards #22063
@Wyverald Wyverald requested a review from fmeum May 9, 2024 21:09
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels May 9, 2024
Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer!

@@ -445,13 +445,14 @@ public static RootModuleFileValue evaluateRootModuleFile(
throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for the root module");
}
for (ModuleExtensionUsage usage : module.getExtensionUsages()) {
if (usage.getIsolationKey().isPresent() && usage.getImports().isEmpty()) {
ModuleExtensionUsage.Proxy firstProxy = usage.getProxies().getFirst();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this into the if and rename to onlyProxy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if itself contains a reference to firstProxy.

@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 9, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 10, 2024
@Wyverald Wyverald deleted the wyv-include-tidy branch May 10, 2024 18:38
Kila2 pushed a commit to Kila2/bazel that referenced this pull request May 13, 2024
The current ModuleExtensionUsage class was designed in the days before we allowed multiple `use_extension` calls for the same extension in the same module. This PR brings the class up to speed to better reflect the actual API.

This will also lay the groundwork for `bazel mod tidy` working with `include()`d segments.

Work towards bazelbuild#22063

Closes bazelbuild#22307.

PiperOrigin-RevId: 632377764
Change-Id: I282a68bc7962088ae4583418f73b2e60a0ec88f0
Wyverald added a commit that referenced this pull request May 13, 2024
The current ModuleExtensionUsage class was designed in the days before we allowed multiple `use_extension` calls for the same extension in the same module. This PR brings the class up to speed to better reflect the actual API.

This will also lay the groundwork for `bazel mod tidy` working with `include()`d segments.

Work towards #22063

Closes #22307.

PiperOrigin-RevId: 632377764
Change-Id: I282a68bc7962088ae4583418f73b2e60a0ec88f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants